-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add password4j implementation of PasswordEncoder #17825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add password4j implementation of PasswordEncoder #17825
Conversation
74cf846 to
ae43ab4
Compare
Here is my PR. I’d really appreciate any feedback or suggestions for improvements.
Thanks for your time and guidance 🙏
@rwinch
ca45b53 to
d9494ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR @mehrdadbozorgmehr!
I've provided feedback inline. Once those get addressed, we will want to discuss updating the documentation.
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
crypto/src/main/java/org/springframework/security/crypto/factory/PasswordEncoderFactories.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
...test/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoderTests.java
Show resolved
Hide resolved
099f552 to
c4d71a5
Compare
f3b431c to
04f994c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress! Thanks again for your PR.
I've added a few inline comments. Please also add documentation to docs/modules/ROOT/pages/features/authentication/password-storage.adoc and docs/modules/ROOT/pages/features/whats-new.adoc
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoder.java
Outdated
Show resolved
Hide resolved
7d67c20 to
8bf697a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we can reuse it as a base class for most of the algorithms, it looks like a single Password4jPasswordEncoder will not be possible. I've provided some feedback inline.
PS: Sorry for finding this late and reverting some of my previous feedback.
...test/java/org/springframework/security/crypto/password4j/Password4jPasswordEncoderTests.java
Outdated
Show resolved
Hide resolved
8bf697a to
fe8626b
Compare
441b4cd to
7daccae
Compare
Thanks for pointing this out. I’ve updated the design by making Password4jPasswordEncoder abstract and introducing algorithm-specific subclasses for the working implementations. I’ve also implemented a PBKDF2-specific encoder and Balloon hashing in this PR with proper salt handling.
@rwinch , please let me know if any additional adjustments are required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes. Overall they look good. Please:
- Rebase to resolve the conflict
- Add documentation including a whats-new.adoc entry that links to the newly added documentation
- Create two tickets:
- For the password4j implementations supporting upgradeEncoding method
- For the password4j implementations being able to match on the same algorithm when the algorithm contains different parameters. This can be done by using the static
getInstanceFromHash(String)method on the implementation (e.g.BcryptFunction.getInstanceFromHash(String)). Note that not all implementations will be able to support this because not all hashes include the parameters in the resulting hash.
7daccae to
a4e6021
Compare
69bcb69 to
ea0a347
Compare
...hm selection and enhance documentation Closes spring-projectsgh-17706 Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com> Add Password4jPasswordEncoder for enhanced password hashing support Signed-off-by: M.Bozorgmehr <m.bozorgmehr@emofid.com> Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com> Add Password4jPasswordEncoder for enhanced password hashing support Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com> Signed-off-by: Mehrdad <mehrdad.bozorgmehr@gmail.com> Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com>
...hm selection and enhance documentation Closes spring-projectsgh-17706 Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com> Signed-off-by: Mehrdad <mehrdad.bozorgmehr@gmail.com> Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com>
...ibrary Closes spring-projectsgh-17706 Signed-off-by: Mehrdad <mehrdad.bozorgmehr@gmail.com> Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com>
...rd encoders using Password4j library Closes spring-projectsgh-17706 Signed-off-by: Mehrdad <mehrdad.bozorgmehr@gmail.com> Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com>
...BCrypt, Scrypt, PBKDF2, and Balloon hashing Closes spring-projectsgh-17706 Signed-off-by: M.Bozorgmehr <mehrdad.bozorgmehr@gmail.com>
ea0a347 to
b2d4c52
Compare
Uh oh!
There was an error while loading. Please reload this page.
Closes gh-17706